-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implement generateCSG method for PinMeshGenerator #31782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7872670 to
d2f3a18
Compare
|
@eshemon @kkiesling FYI |
|
Job Documentation, step Docs: sync website on 6159314 wanted to post the following: View the site here This comment will be updated on new commits. |
d2f3a18 to
762d24e
Compare
|
Job Coverage, step Generate coverage on 6159314 wanted to post the following: Framework coverage
Modules coverageReactor
Full coverage reportsReports
This comment will be updated on new commits. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@GiudGiud this PR should be good to review 🙏🏾 |
GiudGiud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make some sort of "tester" for the CSG, to ensure it is valid
this could be checking volumes? Or simplifying them and making sure a region is not empty? (intersection of +plane and -plane for example) ? or make a converter to OpenMC so the reviewer can plot them there?
modules/reactor/doc/content/source/meshgenerators/PinMeshGenerator.md
Outdated
Show resolved
Hide resolved
modules/reactor/test/tests/meshgenerators/pin_mesh_generator/tests
Outdated
Show resolved
Hide resolved
modules/reactor/test/tests/meshgenerators/pin_mesh_generator/tests
Outdated
Show resolved
Hide resolved
modules/reactor/test/tests/meshgenerators/pin_mesh_generator/gold/pin_hex_3d_csg.json
Outdated
Show resolved
Hide resolved
@GiudGiud Unfortunately, this is not necessarily within scope of the funded work that @shikhar413 and I currently doing. Yes, having some sort of visualization would be helpful as a developer, but that will also naturally come as the connection to specific codes are developed and each code has its native visualization tool that can be used. Additionally, Ideally tests are written in small enough units that the output can be manually inspected by developers/reviewers for correctness (which it seems you were able to do here, noting the mistake), and then they cover enough cases to be able to trust the larger complex models are correct. Also, this CSG framework is not necessarily meant to check the validity of a CSG model, but instead makes sure it matches whatever input it was given. We are operating under the assumption that MOOSE has already validated the input as valid, and therefore if the CSG model is made to match that correctly, it is also valid. So in the example of an empty region, if that is what is created by the MOOSE input and valid there, then that is what would be created and considered valid for CSG as well. Let us know if you want to discuss this in more detail offline/in a meeting. |
762d24e to
a7c8afe
Compare
|
Job Precheck, step Clang format on a7c8afe wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
a7c8afe to
7f225eb
Compare
modules/reactor/test/tests/meshgenerators/pin_mesh_generator/gold/pin_square_3d_csg.json
Show resolved
Hide resolved
7f225eb to
640ef1f
Compare
b45dbf8 to
c173e79
Compare
|
@GiudGiud this should be good for a re-review now |
c173e79 to
60f35e4
Compare
|
@GiudGiud comments should be addressed now. @kkiesling also brought up that we should have a unit test for testing the clone method, so I added that along with equality operator definitions for CSGBase and CSG[Surface|Universe|Cell]List. |
05016ed to
38bc223
Compare
GiudGiud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last comments then good to go
…st, and add a unit test to check equality of cloned CSGBase objects
38bc223 to
6159314
Compare
|
Job Test, step Results summary on 6159314 wanted to post the following: Framework test summaryCompared against 7edd10c in job civet.inl.gov/job/3374418. Added tests
Modules test summaryCompared against 7edd10c in job civet.inl.gov/job/3374418. Added tests
|
|
Thanks @GiudGiud |
Reason
Design
Impact
refs #31733